-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Support for Heated Steering Wheel and Seats #188
Conversation
Heated Seats are off if climate control is off. get seat heat level returns None if climate is off.
Thank you. How does the app handle attempts to change the heaters when the climate is off? Does it automatically enable climate? I think we should try to match it. |
The App automatically turns on the climate. |
Yes. Please try to match the app behavior as a default. |
Just to make sure, this is just waiting for you to let me know it matches the app's behavior by enabling the climate. Are you planning to do that? |
Yeah sorry, i've been snowed under with other stuff at the moment. Its in my plate to get this PR updated so it matches the app. |
Okay, I tried to be patient, but it didn't work. @alandtse would you like me to:
|
Oh i'm sorry! i didn't know anyone else was waiting on this apart from me!! i was going to work on it today anyway, so stand by |
I'm indifferent to the implementation. Most correct would probably be the tesla_device that comes from teslajsonpy. There's a set of HA specific stuff down in the API library. |
We need to wait for HVAC as the heater cannot be turned on while HVAC is off.
No worries man! I was trying to be facetious. It's awesome that you have time today. If not, I'll do it tomorrow. |
Ok, so I've put some new code in here to get the climate device, and use that to enable HVAC - it will wait for up to 30 seconds before it gives up. But I've came up against an interesting bug. Climate Data doesn't appear to be shared across all devices. So if I have the HVAC and heater steering wheel on, then turn off the HVAC in Home Assistant, it can take awhile for the heated steering wheel to be reflected (as an off state). Any ideas on how i could get a shared "climate" state? The climate device doesn't store the raw climate data in the class anywhere, so i can't access it. |
Hmmm. You may have to do some message passing so other entities are aware of linked changes. I don't think there's a specific mechanism otherwise to share state between entities. |
_LOGGER.debug("HVAC Enabled") | ||
return True | ||
else: | ||
_LOGGER.info("Enabing Climate to activate Heated Steering Wheel") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean it will repeatedly send the climate on command every second for up to 30s?
Wouldn't you rather send it once and wait? Or is it sometimes flaky, and the 30 retries be potentially useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is probably not the best way to implement because it will spam the api. Thinking about this more, it's probably much cleaner to have a single api command that sends the hvac start command, checks ever few seconds to see if it's been enabled, and then issues the second command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really feels like the wakeup decorator. Instead of waking up the car, it instead is enabling the hvac.
…eated-Steering-Wheel. Heated Seats and Heated Steering Wheel will now share logic
Ok, did some more work on this. There's some code in there that i'm kinda not proud of - it feels dirty, but as explained in code comments, its the only way I could force other devices to update based on the climate device. I'm fully aware that setting a double underscored class variable externally to the class is really not good python code. haha. This really highlighted the need to remove any home assistant stuff from the underlying library, so we can have all the code that influences home assistant logic in a single library. I'd be really interested in knowing your thoughts here. |
# This is really gross, and i kinda hate it. | ||
# but its the only way i could figure out how to force an update on the underlying device | ||
# thats in the teslajsonpy library. | ||
# This could be fixed by doing a pr in the underlying library, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By adding a public method to allow resetting the manual update time?
Perhaps it's worth filing an issue on the teslajsonpy
repo, so this doesn't get forgotten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's open an issue. Basically what you're saying we need is a way to force a device to reread its info from cache.
I'm ok with the current approach for a 1.0 type release, but it makes sense to resolve it more cleanly in the future.
# So it could eat into our timeout, and this is fine. | ||
# We'll try to turn the set the status, and check again | ||
try: | ||
await climate_device.set_status(True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often on your system does this hit the Tesla endpoint? The issue I'm concerned about is that each api call has a wake call which includes 5 separate retries. Since this loop in theory can hit 15 times and if there's a wakeup that quickly goes to 75 api hits. Are we slamming the endpoint unnecessarily?
# This is really gross, and i kinda hate it. | ||
# but its the only way i could figure out how to force an update on the underlying device | ||
# thats in the teslajsonpy library. | ||
# This could be fixed by doing a pr in the underlying library, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's open an issue. Basically what you're saying we need is a way to force a device to reread its info from cache.
I'm ok with the current approach for a 1.0 type release, but it makes sense to resolve it more cleanly in the future.
Ok, combining your feedback and more testing, I've more tightly controlled when we go to the Tesla API for an update, and forcing other devices to read from cache. We're actually a little behind on feature parody with tesla's app, and i this this is due to the need to update 2 libraries to implement a new feature. |
Great. This looks better. Can you rebase off dev so I can then merge this in please? We already have that issue to do the separation. It's just been a lack of motivation to be honest. zabuldon/teslajsonpy#24 |
Heated Seats are off if climate control is off. get seat heat level returns None if climate is off.
We need to wait for HVAC as the heater cannot be turned on while HVAC is off.
8300989
to
bfd8cb5
Compare
I think we're in a good place now for a merge into dev. |
FYI @Megabytemb you need to use a dict.get(). See #199 |
Created |
Add Heated Steering Wheel Switch
Allows users to toggle the Heated Steering Wheel from Home Assistant.
This has a silent restriction where it only works when the Climate is Enabled within a car (Similar to the Heated Seats).
If climate is not enabled, then the switch silently fails.